Core: Sequence bounds preconditions#1100
Conversation
dacef58 to
5e268e5
Compare
5e268e5 to
1925770
Compare
atomb
left a comment
There was a problem hiding this comment.
This looks nice. There's a little room to reduce repetition, however.
1925770 to
bbf0d27
Compare
tautschnig
left a comment
There was a problem hiding this comment.
Proof-coverage suggestions ("what to prove next"). These are all follow-ups, not blocking.
-
convertMetaDataPropertyType_inverse_of_classifyPrecondition— for every(fn, idx)thatclassifyPreconditionmaps tosome metaString,convertMetaDataPropertyTypeapplied to aMetaDatatagged with that string returns a non-.assertPropertyType. Concretely: there shouldn't be a gap whereclassifyPreconditiontags a call butconvertMetaDataPropertyTypesilently reverts to.assert. Trivial to state, would catch the string-mismatch class of regressions the triplication enables. -
PropertyType-to-SARIF round-trip —(propertyTypeToClassification p).fromClassification? = some pfor allp : PropertyType, wherefromClassification?is a matching deserializer (not yet written). Would makePropertyTypethe single source of truth and mechanically prune theMetaData.*string constants. -
mkSeqBoundsPrecond_form— a structural theorem about(mkSeqBoundsPrecond x k).expr: it equalsboolAnd (intLe 0 x) (k.opExpr x (seqLength s))(or the concrete LExpr shape). MakesTest 10's off-by-one blind spot (see inline comment #2) explicit and proved rather than merely tested. Small. -
seqSelect_denote_partial— semantic soundness of the bounds precondition with respect to Lean'sList.get?:0 ≤ i < s.length → (seqSelect s i).denote = .some (s.get! i). This isFactoryCorrect.leanterritory (cf. PR #1103) rather than this PR, but worth tracking as the obvious continuation: the preconditions added here are only useful insofar as they correspond to a semantic partiality. -
collectPrecondAsserts-spec — a purely structural theorem that for every sub-expression that is a call to a function withNpreconditions,collectPrecondAssertsproduces exactlyNassertstatements, each tagged byclassifyPrecondition. Test 10 verifies counts 1, 1, 1, 1, and 2 empirically; a theorem would subsume all of that and catch e.g. a future refactor that stops visiting nested calls.
Refactoring opportunities (not blocking).
- As noted in finding #3, consolidate
MetaData.{divisionByZero,...}/propertyTypeToClassification/convertMetaDataPropertyTypeinto a single table. - The new
assertOutOfBoundsObligationshelper in the tests is a good pattern; the pre-existing overflow/division tests in the same file (Test 1,Test 3,Test 5) could share a sibling helper, reducing per-test boilerplate uniformly.
MikaelMayer
left a comment
There was a problem hiding this comment.
🤖 Clean PR. The convertMetaDataPropertyType extraction is a good de-duplication, SeqBoundKind is a nice way to prevent nested-precondition accidents, and the SARIF property-type wiring fix is a genuine improvement (was dead code before). Test coverage is solid.
No new issues beyond what's already been flagged in existing threads.
bbf0d27 to
2bf64c3
Compare
8325c0e to
dca23bd
Compare
Add `seqName : String := "s"` parameter so the helper no longer hardcodes
the input variable name in a string literal three definitions away from
the call sites that depend on it. All four current callers (`Sequence.{select,update,take,drop}`) name their `Sequence a` input `"s"` and use the
default. A future partial Sequence op with a different input name need
only pass it explicitly rather than rely on the hidden literal.
Mismatches between a function's declared inputs and the names used in its
preconditions remain caught at elaboration by `polyUneval`'s `h_precond`
free-vars check; this change only reduces the code-distance between
cause and effect.
Cherry-pick target: PR #1100. The change sits inside #1100's
`mkSeqBoundsPrecond`, so it belongs there. When #1100 lands and this
branch rebases on main, the commit will be deduplicated automatically.
Add `seqName : String := "s"` parameter so the helper no longer hardcodes
the input variable name in a string literal three definitions away from
the call sites that depend on it. All four current callers (`Sequence.{select,update,take,drop}`) name their `Sequence a` input `"s"` and use the
default. A future partial Sequence op with a different input name need
only pass it explicitly rather than rely on the hidden literal.
Mismatches between a function's declared inputs and the names used in its
preconditions remain caught at elaboration by `polyUneval`'s `h_precond`
free-vars check; this change only reduces the code-distance between
cause and effect.
Cherry-pick target: PR #1100. The change sits inside #1100's
`mkSeqBoundsPrecond`, so it belongs there. When #1100 lands and this
branch rebases on main, the commit will be deduplicated automatically.
e2e5870 to
24db579
Compare
MikaelMayer
left a comment
There was a problem hiding this comment.
🤖🔍 All previous comments appear addressed — reviewer sign-off still needed.
polyUneval is the combinator used to declare unevaluated polymorphic functions with axioms. Unlike unaryOp and binaryOp, it had no way to attach preconditions; callers had to hand-build the WFLFunc. Add a 'preconditions' parameter and the matching free-vars proof obligation (subset of the function's input names), defaulting to empty. No behavioral change for existing callers.
Sequence.select and Sequence.update now require `0 <= i < length(s)`; Sequence.take and Sequence.drop require `0 <= n <= length(s)`. PrecondElim picks these up and generates VC obligations at call sites, both in statement positions (via transformStmt) and in pure positions (via mkContractWFProc / mkFuncWFProc) — so requires/ensures/quantifier-body subscripts are also covered. Obligations carry the propertyType metadata "outOfBoundsAccess" (new MetaData constant) and flow through a new PropertyType.outOfBoundsAccess enum variant — with matching entries in the statement-eval / obligation-extraction / cmd-eval metadata-to-PropertyType conversion sites — to finally render as "out-of-bounds-access" in SARIF output, matching how divisionByZero and arithmeticOverflow are classified. Side effect: `propertyTypeToClassification` in SarifOutput.lean was previously dead code; `vcResultToSarifResult` never set `properties.propertyType` so the SARIF output defaulted every obligation to "assert". Wiring this up means divisionByZero and arithmeticOverflow obligations now also classify correctly in SARIF — a pre-existing bug this PR incidentally fixes.
New tests in StrataTest/Transform/PrecondElim.lean:
- Test 10: Sequence.select in a procedure body emits the bounds assert
(PrecondElim is unconditional — it inserts regardless of any
surrounding requires guard; the SMT solver discharges).
- Test 10c: Sequence.select inside a requires clause triggers the
$$wf-procedure path (mkContractWFProc).
- Test 10d: Sequence.select inside a function body triggers the
function-body $$wf path (mkFuncWFStmts).
- Test 11: collectPrecondAsserts attaches outOfBoundsAccess metadata
for all four partial ops and a nested call. Mirrors
OverflowCheckTest.lean. Also verifies Sequence.length emits
no obligation (it is total).
New property-classification tests in
StrataTest/Languages/Core/Tests/SarifOutputTests.lean cover all five
PropertyType variants, exercising the SARIF wiring fix in commit 3.
Collateral test updates for real behavioral changes:
- StrataTest/Languages/Core/Examples/Seq.lean: expected VC output
includes the new bounds obligations (all SMT-provable from the
surrounding context, except the pre-existing contains_yes unknown).
- StrataTest/Languages/Core/Tests/ProgramEvalTests.lean: Sequence func
signatures now render with the attached requires clauses.
Add `seqName : String := "s"` parameter so the helper no longer hardcodes
the input variable name in a string literal three definitions away from
the call sites that depend on it. All four current callers (`Sequence.{select,update,take,drop}`) name their `Sequence a` input `"s"` and use the
default. A future partial Sequence op with a different input name need
only pass it explicitly rather than rely on the hidden literal.
Mismatches between a function's declared inputs and the names used in its
preconditions remain caught at elaboration by `polyUneval`'s `h_precond`
free-vars check; this change only reduces the code-distance between
cause and effect.
Cherry-pick target: PR #1100. The change sits inside #1100's
`mkSeqBoundsPrecond`, so it belongs there. When #1100 lands and this
branch rebases on main, the commit will be deduplicated automatically.
24db579 to
2ab3b4b
Compare
MikaelMayer
left a comment
There was a problem hiding this comment.
🤖🔍 All previous comments appear addressed — reviewer sign-off still needed.
Blocker for Laurel Seq/Array in #1073.
Bounds preconditions for
Sequence.select/update/take/dropFollowing the
Int.SafeDivpattern:Sequence.select0 <= i && i < Sequence.length(s)Sequence.update0 <= i && i < Sequence.length(s)Sequence.take0 <= n && n <= Sequence.length(s)Sequence.drop0 <= n && n <= Sequence.length(s)Sequence.length/empty/append/build/containsremain total.PrecondElimpicks the obligations up automatically at call sites in imperative code (viatransformStmt) and in pure positions likerequires/ensures/ quantifier bodies / function bodies (via the synthetic$$wfprocedures).Obligations carry
propertyType = "outOfBoundsAccess"(newMetaDataconstant, mirroringdivisionByZero), flow through a newPropertyType.outOfBoundsAccessenum variant and the three metadata-to-PropertyType conversion sites, and render as"out-of-bounds-access"in SARIF output.Incidental fix
While wiring the SARIF classification, I noticed
propertyTypeToClassificationinSarifOutput.leanwas pre-existing dead code:vcResultToSarifResultnever setproperties.propertyType, so every obligation defaulted to"assert"in SARIF output. This PR wires it up, sodivisionByZeroandarithmeticOverflowobligations now also classify correctly in SARIF alongside the newoutOfBoundsAccess.Testing
StrataTest/Transform/PrecondElim.leanTest 10 —collectPrecondAssertsattachesoutOfBoundsAccessmetadata for all four partial ops plus a nestedSequence.select(Sequence.update(...))call. Mirrors the pattern inOverflowCheckTest.lean. Complemented by per-op pretty-print snapshots (#guard_msgs) that assert the exact obligation body string — these catch regressions that preserve count and metadata tag but corrupt the obligation body (e.g. passing.Leinstead of.LttomkSeqBoundsPrecondat a call site, changing the bound-variable name, or swapping the lower/upper bound insidemkSeqBoundsPrecond).StrataTest/Languages/Core/Tests/SarifOutputTests.lean— property-type classification tests covering all fivePropertyTypevariants.Collateral updates to existing tests reflect the new obligations (
Seq.lean) and updatedrequireson Sequence function signatures (ProgramEvalTests.lean). Note: the bounds obligations inSeq.leanappear astrue && 0 < length(...)— the partial evaluator simplifies0 <= 0totruebut does not further simplifytrue && XtoX. This is a pre-existing evaluator gap, not newly introduced by this PR; the SMT solver discharges the obligation trivially.Known downstream impact
PR #1073 (Laurel Seq/Array) emits
Sequence.select/update/take/dropcalls from its translation. ItsT18_Sequencestest uses#guard_msgs(drop info, error)on a diagnostics-only pipeline, so the test assertions should still pass syntactically — but individual sequence-manipulation programs now require bounds guards to fully verify. PR #1073 has been adjusted accordingly.